Skip to content

Implement --check-stack-overflow flag for wasm-emscripten-finalize#2278

Merged
quantum5 merged 9 commits intoWebAssembly:masterfrom
quantum5:safe-stack
Aug 2, 2019
Merged

Implement --check-stack-overflow flag for wasm-emscripten-finalize#2278
quantum5 merged 9 commits intoWebAssembly:masterfrom
quantum5:safe-stack

Conversation

@quantum5
Copy link
Copy Markdown
Member

@quantum5 quantum5 commented Aug 1, 2019

When --check-stack-overflow is passed to wasm-emscripten-finalize, a function called __set_stack_limit is exported. Calling this function with the low end of the stack will signal to the module how far the stack is allowed to grow.

When the stack pointer dips below the value set by __set_stack_limit, an imported function __handle_stack_overflow is called.

If __set_stack_limit is not called, then stack overflow check is disabled.

Refs emscripten-core/emscripten#9039.

Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel I am missing the big picture here, as I would expect that you just check the values assigned to the stack pointer to see if they are within range. Instead I see some local tracking and canBeSubtract etc. which seems to do something more complex and specific..?

Comments might have helped avoid my confusion :)

}
wasm.updateMaps();

if (checkStackOverflow && !isSideModule) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of silently doing nothing for side modules, how about Fatal() << "stack overflow checks are not supported with side modules"; ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we actually need to do nothing for side modules because all stack operations gets turned into stackSave and stackRestore, which are exported from the main module. In the main module, stackRestore performs the overflow check.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that right? I thought side modules manage their own stack in their own memory. At least that's how fastcomp does it - is it completely different in the wasm backend?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it. This is the prologue of a function in the side module:

 (func $f (; 5 ;) (type $2) (result i32)
  (local $0 i32)
  (local $1 i32)
  (local $2 i32)
  ...
  (local.set $0
   (call $stackSave)
  )
  (local.set $1
   (i32.const 8192)
  )
  (local.set $2
   (i32.sub
    (local.get $0)
    (local.get $1)
   )
  )
  (call $stackRestore
   (local.get $2)
  )

and this is the epilogue:

  (local.set $7
   (i32.const 8192)
  )
  (local.set $8
   (i32.add
    (local.get $2)
    (local.get $7)
   )
  )
  (call $stackRestore
   (local.get $8)
  )

stackSave and stackRestore are imported:

 (import "env" "stackSave" (func $stackSave (result i32)))
 (import "env" "stackRestore" (func $stackRestore (param i32)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very surprising. Let's see what @sbc100 says. But that doesn't need to block this PR landing (we may need a followup though).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this surprising? The sp with the wasm backend is mutable global but we don't have any way to import or export mutable globals (yet), so the side module has to manipulate the global via these helper functions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks @sbc100

I guess I'm used to the fastcomp model where not just threads but shared modules also got their own stack.

Comment thread src/wasm/wasm-emscripten.cpp Outdated
Comment thread src/wasm/wasm-emscripten.cpp Outdated
@quantum5
Copy link
Copy Markdown
Member Author

quantum5 commented Aug 2, 2019

The local tracking and other stuff are there to avoid an additional check on the function epilogue, which should be incapable of causing a stack overflow.

@kripken
Copy link
Copy Markdown
Member

kripken commented Aug 2, 2019

I see, thanks. I think it's better to just check every assign to the stack pointer. This is a debugging tool so optimizing every bit of perf seems less important than clarity and simplicity.

Comment thread src/wasm/wasm-emscripten.cpp Outdated
@kripken
Copy link
Copy Markdown
Member

kripken commented Aug 2, 2019

My question about side modules was marked "resolved" but what's the answer? :)

Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm aside from the side module issue

@quantum5 quantum5 merged commit 4f0d960 into WebAssembly:master Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants